From 88f5bb5a2d9f9a670d97ab0853f13a7e3b5a1bb0 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Mon, 27 Jul 2015 15:40:02 -0700
Subject: [PATCH] Remove an Rc optimization that's no longer necessary

This optimization ended up not being correct with the recent switch to being
more recursive, and after some profiling it looks like this optimization for
memory usage isn't even needed any more. This commit removes the `Rc` sharing,
fixing #1841 in the process.

Closes #1841
---
 src/cargo/core/resolver/mod.rs | 15 +++++++--------
 tests/test_cargo_registry.rs   | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs
index c7c245b2c..0fe74b8d8 100644
--- a/src/cargo/core/resolver/mod.rs
+++ b/src/cargo/core/resolver/mod.rs
@@ -92,7 +92,6 @@
 //! dependency graph (one of the largest known ones), so hopefully it'll work
 //! for a bit longer as well!
 
-use std::cell::RefCell;
 use std::collections::HashSet;
 use std::collections::hash_map::HashMap;
 use std::fmt;
@@ -232,7 +231,7 @@ impl fmt::Debug for Resolve {
 struct Context {
     activations: HashMap<(String, SourceId), Vec<Rc<Summary>>>,
     resolve: Resolve,
-    visited: Rc<RefCell<HashSet<PackageId>>>,
+    visited: HashSet<PackageId>,
 }
 
 /// Builds the list of all packages required to build the first argument.
@@ -244,7 +243,7 @@ pub fn resolve(summary: &Summary, method: Method,
     let cx = Box::new(Context {
         resolve: Resolve::new(summary.package_id().clone()),
         activations: HashMap::new(),
-        visited: Rc::new(RefCell::new(HashSet::new())),
+        visited: HashSet::new(),
     });
     let _p = profile::start(format!("resolving: {}", summary.package_id()));
     match try!(activate(cx, registry, &summary, method, &mut |cx, _| Ok(Ok(cx)))) {
@@ -271,14 +270,14 @@ fn activate(mut cx: Box<Context>,
     // Dependency graphs are required to be a DAG, so we keep a set of
     // packages we're visiting and bail if we hit a dupe.
     let id = parent.package_id();
-    if !cx.visited.borrow_mut().insert(id.clone()) {
+    if !cx.visited.insert(id.clone()) {
         return Err(human(format!("cyclic package dependency: package `{}` \
                                   depends on itself", id)))
     }
 
     // If we're already activated, then that was easy!
     if cx.flag_activated(parent, &method) {
-        cx.visited.borrow_mut().remove(id);
+        cx.visited.remove(id);
         return finished(cx, registry)
     }
     debug!("activating {}", parent.package_id());
@@ -292,8 +291,8 @@ fn activate(mut cx: Box<Context>,
     };
 
     activate_deps(cx, registry, parent, platform, deps.iter(), 0,
-                  &mut |cx, registry| {
-        cx.visited.borrow_mut().remove(parent.package_id());
+                  &mut |mut cx, registry| {
+        cx.visited.remove(id);
         finished(cx, registry)
     })
 }
@@ -373,7 +372,7 @@ fn activate_deps<'a>(cx: Box<Context>,
         // If we hit an intransitive dependency then clear out the visitation
         // list as we can't induce a cycle through transitive dependencies.
         if !dep.is_transitive() {
-            my_cx.visited.borrow_mut().clear();
+            my_cx.visited.clear();
         }
         let my_cx = try!(activate(my_cx, registry, candidate, method,
                                   &mut |cx, registry| {
diff --git a/tests/test_cargo_registry.rs b/tests/test_cargo_registry.rs
index 4dc750a99..9f9426c7a 100644
--- a/tests/test_cargo_registry.rs
+++ b/tests/test_cargo_registry.rs
@@ -771,3 +771,37 @@ test!(update_transitive_dependency {
 {compiling} foo v0.5.0 ([..])
 ", downloading = DOWNLOADING, compiling = COMPILING)));
 });
+
+test!(update_backtracking_ok {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.5.0"
+            authors = []
+
+            [dependencies]
+            webdriver = "0.1"
+        "#)
+        .file("src/main.rs", "fn main() {}");
+    p.build();
+
+    r::mock_pkg("webdriver", "0.1.0", &[("hyper", "0.6", "normal")]);
+    r::mock_pkg("hyper", "0.6.5", &[("openssl", "0.1", "normal"),
+                                    ("cookie", "0.1", "normal")]);
+    r::mock_pkg("cookie", "0.1.0", &[("openssl", "0.1", "normal")]);
+    r::mock_pkg("openssl", "0.1.0", &[]);
+
+    assert_that(p.cargo("generate-lockfile"),
+                execs().with_status(0));
+
+    r::mock_pkg("openssl", "0.1.1", &[]);
+    r::mock_pkg("hyper", "0.6.6", &[("openssl", "0.1.1", "normal"),
+                                    ("cookie", "0.1.0", "normal")]);
+
+    assert_that(p.cargo("update").arg("-p").arg("hyper"),
+                execs().with_status(0)
+                       .with_stdout(&format!("\
+{updating} registry `[..]`
+", updating = UPDATING)));
+});
-- 
2.30.2